@jd-alexander This is looking pretty good already -- nice work! I know it's early days (and this is Android-only atm) but I wanted to chime in with a couple of design-related bits of feedback:

2. Cancel option in the sheet removes the Image placeholder

Now that I see this in context, I think we might not want to lean towards keeping the placeholder in this scenario. For example, it seems like a scenario (more likely than exiting the flow right away) would be where I have created the block but want to "skip" the upload part -- perhaps because I don't have an image ready or want to leave it as a blank image block (think: templates).

The tricky part with that is what does the label become (not "cancel")? in this flow, something like "Skip" or "Skip upload"might make sense, but we do use on this sheet in other contexts so something more generic like "Dismiss" or "Keep editing" would probably be more appropriate. I'm also open to other suggestions 😄

• Regardless of what that label is, I'd like to add a separator between the last source option ("choose from tenor" in most cases) and the "cancel"/"dismiss" option (using the same spacing metrics that are used on the "clear media" button in your last example)

• I don't know if we need to do anything about this one in this specific ticket, but the inserter sheet closes abruptly upon selection, resulting in a bit of an awkward delay in between when that closes and the media-source sheet opens. I think this "abruptness" is a known issue that also affects various other sheets -- such as your examples above where you tap "clear media" and "cancel". It'd be nice to solve that, but I think it's something that would be better addressed across the board. // cc @dratwas

@jd-alexander This is looking pretty good already -- nice work! I know it's early days (and this is Android-only atm) but I wanted to chime in with a couple of design-related bits of feedback:

Thank a lot for the feedback @iamthomasbishop 🙇

Cancel option in the sheet removes the Image placeholder
Now that I see this in context, I think we might not want to lean towards keeping the placeholder in this scenario. For example, it seems like a scenario (more likely than exiting the flow right away) would be where I have created the block but want to "skip" the upload part -- perhaps because I don't have an image ready or want to leave it as a blank image block (think: templates).

Just to clarify here, "I think we might not want to lean towards keeping the placeholder in this scenario" I am trying to understand this point if you are saying that the way the cancel option was implemented is correct or if "not" being included was a typo. Let me know :)

The tricky part with that is what does the label become (not "cancel")? in this flow, something like "Skip" or "Skip upload"might make sense, but we do use on this sheet in other contexts so something more generic like "Dismiss" or "Keep editing" would probably be more appropriate. I'm also open to other suggestions 😄

This is a good point. I would choose "Dismiss" as it clearly communicates that the component in question is going to be closed.

Regardless of what that label is, I'd like to add a separator between the last source option ("choose from tenor" in most cases) and the "cancel"/"dismiss" option (using the same spacing metrics that are used on the "clear media" button in your last example)

Will add this 🙏

I don't know if we need to do anything about this one in this specific ticket, but the inserter sheet closes abruptly upon selection, resulting in a bit of an awkward delay in between when that closes and the media-source sheet opens. I think this "abruptness" is a known issue that also affects various other sheets -- such as your examples above where you tap "clear media" and "cancel". It'd be nice to solve that, but I think it's something that would be better addressed across the board. // cc @dratwas

Indeed. I have noticed this as well. I will check to see if there are any existing issues related to this and if not I will track it.

Hi @dratwas 👋 based on the comment above in which you are mentioned I went ahead and did a search of our issues and saw this issue #24060 I saw that there was an associated PR as well so I am wondering if this issue is still able to track this.

@mchowning @guarani just a heads up that there's a bug on iOS that I am currently investigating causing the media upload option bottom sheet to be closed as it opens so I am investigating that today. The functionality here works as expected on Android. Once I get a first pass from you peeps with the approach and some thoughts on the questions posed, I will work on finalizing the PR for an official review. Thanks in advance 😄

On second thought I have been doing some debugging with iOS and I can't figure out why after adding an image block multiples with this functionality the editor freezes. I see no exceptions so I am still digging to figure out what's up. So it would be good if either of the reviewers could run to the iOS emulator for more context.

Not directly related to this PR, but thought I'd mention: I just discovered how to locally checkout a PR made on a fork. So for this PR it's:

git fetch origin pull/25031/head:rnmobile/simplify_image_insertion_flow
git checkout rnmobile/simplify_image_insertion_flow

Not directly related to this PR, but thought I'd mention: I just discovered how to locally checkout a PR made on a fork. So for this PR it's:

git fetch origin pull/25031/head:rnmobile/simplify_image_insertion_flow
git checkout rnmobile/simplify_image_insertion_flow

Nice! That's pretty cool :)

I had a look at the code and it's looking great. On iOS I see the same bug you reported here #25031 (comment) (I don't have any ideas off-hand on what's causing the media options sheet to disappear.)

I wanted to test on Android but it seems like something's broken in my setup and I couldn't get the emulator to load the app from the packager (it seemed to use a bundled version of the app — I don't recall seeing this problem before when running from Gutenberg 🤷‍♂️). It's very likely something with my setup but I couldn't fix it yet.

If you click Cancel on the media options sheet, then the Image block will be removed. I haven't figured out how to actually edit the history so that undo doesn't bring the Image block back. So currently, when you press undo it's restored and the sheet is opened. Also, I want to know what your thoughts are on clicking Cancel and the block being removed since the primary way to remove blocks is through the block options. If we are proceeding with this implementation, should dismissing the options sheet also cause the block to be removed as well? I have this logic in another branch, where the Picker component in both iOS and Android is modified to expose it's onClosed event.

Since I haven't had a chance to test this properly, so not sure.

Since the Cover and Image blocks share the same image settings button, the Clear Media logic was shared between both. This current implementation removes the ability of passing in the picker options. 7de89e0

I wasn't sure about this as it seemed to limit customization (and see my comment #25031 (comment)).

Since the Video block is similar to the Image block should this PR also address those changes there?

My feeling is that Video should only be included if it's easier to include than exclude (e.g. due to shared code), doesn't have any separate design considerations, and it doesn't make the test scenarios in this PR unreasonably long (in which case it could go in another PR).

Just to clarify here, "I think we might not want to lean towards keeping the placeholder in this scenario"

@jd-alexander Oops, my bad -- yes, I think we should keep the placeholder, and change the Cancel button to Dismiss or Keep editing. 😄 And I think Keep editing or Skip would be a little more intuitive in this specific scenario (right after I've added the block), while Dismiss would be a nice generic label to cover most other cases. If we have to stick with one label across the board, then let's roll with Dismiss.

Will add this 🙏

Thank you!

Thanks again for the review @guarani 🙌

On iOS I see the same bug you reported here #25031 (comment) (I don't have any ideas off-hand on what's causing the media options sheet to disappear.)

So I spent some time investigating this and it seems that the media options action sheet on iOS is showing too quickly, so the inserter bottom sheet being dismissed at the same time is causing it to be closed. I added a delay() to the function that presents the action sheet/ Picker and that resolved the issue. It would be better if I had some callback or way of listening to the onClose event of the bottom sheet because I am not a big fan of using delay() with UI components but I am not sure if that can be done easily without leaking responsibilities from one component into another so I will utilize the delay() when I continue this PR after HACK Week and let you know how it goes :)

I wanted to test on Android but it seems like something's broken in my setup and I couldn't get the emulator to load the app from the packager (it seemed to use a bundled version of the app — I don't recall seeing this problem before when running from Gutenberg 🤷‍♂️). It's very likely something with my setup but I couldn't fix it yet.

Understood :)

I wasn't sure about this as it seemed to limit customization (and see my comment #25031 (comment)).

Yes, indeed. It does limit customization.

My feeling is that Video should only be included if it's easier to include than exclude (e.g. due to shared code), doesn't have any separate design considerations, and it doesn't make the test scenarios in this PR unreasonably long (in which case it could go in another PR).

Thanks for this. I will evaluate including it here or not when I finalize the changes here 🙏

@guarani this is ready for another review. The same testing instructions in the main PR description applies here. All the issues you had mentioned in a previous review are resolved now since this approach is not state-based.

The primary success paths of this feature are as follows.

1. When you open the Image, Video, or Gallery Block, you should be prompted with a modal to select a media source and add media to the respective block.

2. When you copy an empty media block and paste it, it opens the sheet. I think this behavior is fine since you are following the same inserter flow to add the block and if you copied an empty block, I think it's fine to assume you are going to want it opened. If this behavior seems unneeded in anyway I could try to find a way to listen for the copy button/action and somehow mark a block as copied and store that state but I haven't investigated how to do that as yet as the thought just came to me.

Also, thanks for your initial thoughts and ideas about modifying the inserter 💯 I have a function that's updating the redux state when an insertion takes place and any block that's inserted in this can listen for it.

Quick question, if behavior like the above is being introduced should it go into some form of documentation or it's fine to just submit as is? I am going to look into this.

Hi @chipsnyder just an update here, that this PR that simplifies the media flow for the Image, Video, and Gallery block was revisited and updated for review. From a review standpoint, most of the changes got reviewed by @guarani in the past but that was a while ago, so hopefully, if you are available, you could take another look @guarani Also, if necessary, someone from manakin could also take a look since this PR touches critical flows for our media blocks.

Closing this PR in favor of #29546 that splits up the changes into smaller PRs so that they can be reviewed more easily. Changes not included in the PR series are the changes the addition of Clear Media to the Image edit options and the Cancel to Dismiss label change in the picker. Those changes will be seperate/isolated PRs, if necessary since they aren't related to the main goal of this PR. The Gutenberg Mobile PR, will still be utilized with this PR series, where the Gutenberg ref will be updated once all the changes have been merged into the base PR and then there is a merge commit of the merge into trunk.